Skip to content

Zero-fee commitments support#660

Open
tankyleo wants to merge 29 commits into
lightningdevkit:mainfrom
tankyleo:25-10-0fc-channel-config
Open

Zero-fee commitments support#660
tankyleo wants to merge 29 commits into
lightningdevkit:mainfrom
tankyleo:25-10-0fc-channel-config

Conversation

@tankyleo

@tankyleo tankyleo commented Oct 13, 2025

Copy link
Copy Markdown
Contributor

No description provided.

@ldk-reviews-bot

ldk-reviews-bot commented Oct 13, 2025

Copy link
Copy Markdown

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@tankyleo tankyleo requested a review from tnull October 13, 2025 13:14
Comment thread src/config.rs
/// `option_anchor_zero_fee_commitments`. All the caveats and warnings in
/// [`AnchorChannelsConfig`] still apply.
/// [`AnchorChannelsConfig`]: Config::anchor_channels_config
pub enable_zero_fee_commitments: bool,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we'll wan to add a new flag here that's probably hard to understand for the user? Rather, shouldn't we enable this for the user based on our current 'trust model settings' here?

Also, from these docs it's very unclear what this setting even does, when the user would want to enable it, what drawbacks it has, etc

@tnull tnull Oct 13, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, thinking about it again it seems that we should never set negotiate_anchor_zero_fee_commitments until we're positive our chain sources support submitpackage/TRUC, no? And once we are positive, we would always set it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather, shouldn't we enable this for the user based on our current 'trust model settings' here?

Don't quite follow here could you expand ? I think 0FC channels merit an explicit setting somewhere rather than derived from trust model settings.

Also, from these docs it's very unclear what this setting even does, when the user would want to enable it, what drawbacks it has, etc

Yes will expand

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't quite follow here could you expand ? I think 0FC channels merit an explicit setting somewhere rather than derived from trust model settings.

Why, what do they fundamentally change for the user compared to our three current modes (fully trusted/keep 0-reserve, still try to claim/keep X reserve, try to claim)? Keep in mind that communicating these three modes to the user is already very hard, they always have a very hard time understanding what this means. Now, how would we communicate any changed assumptions for 0FC here? If we already trust our counterparty already, wouldn't we always want to enable 0FC for the UX improvements?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why, what do they fundamentally change for the user compared to our three current modes (fully trusted/keep 0-reserve, still try to claim/keep X reserve, try to claim)?

Let me see I don't think they change anything ? Whether to enable or disable 0FC channels is orthogonal to these modes ie trusted_peers_no_reserve and per_channel_reserve_sats should have no influence on whether we enable 0FC channels (only that per_channel_reserve_sats should be set to some value). I suspect you don't agree :)

If we already trust our counterparty already, wouldn't we always want to enable 0FC for the UX improvements?

It seems to me trusting our counterparty -> keeping 0 reserve is orthogonal to whether the user wants to enable 0FC channels ? for example a user trusts their counterparty, but wants to wait for greater adoption of Core v29+ before using 0FC channels.

@tankyleo tankyleo force-pushed the 25-10-0fc-channel-config branch from cb1cdf9 to c874049 Compare October 24, 2025 06:05
@tankyleo tankyleo requested a review from tnull October 24, 2025 06:06
@tankyleo tankyleo marked this pull request as draft October 24, 2025 06:06
@tankyleo

Copy link
Copy Markdown
Contributor Author

Marked as draft: I think we should wait for electrum and esplora submit package support before merging this PR.

@tankyleo tankyleo force-pushed the 25-10-0fc-channel-config branch from c874049 to ef3ba7a Compare October 27, 2025 05:15
@tankyleo

tankyleo commented Oct 27, 2025

Copy link
Copy Markdown
Contributor Author

Successfully opened some 0FC channels, made payments, and force closed them with the esplora diff in this branch.

https://mutinynet.com/tx/508a954d85f5b7daf224a2fdc54ea6de9a26c0f62f7d58284bf61c3cdfd346e6

@tankyleo tankyleo force-pushed the 25-10-0fc-channel-config branch from ef3ba7a to 3ebd017 Compare October 29, 2025 08:56
@tankyleo tankyleo changed the title Add AnchorChannelsConfig::enable_zero_fee_commitments Zero-fee commitments support Oct 30, 2025
@tankyleo tankyleo self-assigned this Oct 30, 2025
@tankyleo tankyleo force-pushed the 25-10-0fc-channel-config branch from 3ebd017 to eda13d4 Compare November 11, 2025 04:16
@tankyleo tankyleo removed this from Weekly Goals Nov 12, 2025
@tankyleo tankyleo force-pushed the 25-10-0fc-channel-config branch 4 times, most recently from e136f33 to d4a2a04 Compare November 19, 2025 19:11
@tankyleo tankyleo force-pushed the 25-10-0fc-channel-config branch 9 times, most recently from 771f45b to a7c7911 Compare May 29, 2026 00:36
@tankyleo tankyleo marked this pull request as ready for review May 29, 2026 01:08
@tankyleo tankyleo requested a review from benthecarman May 29, 2026 01:08
tankyleo added 26 commits July 15, 2026 06:10
When we are preparing to open a channel to a peer, we should reserve
onchain funds for an anchor channel when the peer's init features
signals anchor channels as optional, as channel negotiation with such a
peer can result in an anchor channel.

Tests written with codex.
We previously allowed users to disable anchor channels and drain their
anchor reserve while still having anchor channels open or pending
resolution. This was acceptable for keyed anchor channels, as the
commitment transaction therein still contained some fees, and had some
chance of getting mined into a block without any anchor bumps.

In upcoming commits, we will add support for 0FC channels, and their
commitment transactions have zero fees and depend entirely on the anchor
reserve to reach miners and get confirmed in a block. It is thus
dangerous to disable anchor channels and drain the reserve after 0FC
channels have been opened. Therefore, we make `AnchorChannelsConfig`
required, and prevent this case from ever happening.
The patch adds support for the `broadcast_package` method added in
electrum protocol v1.6. Upcoming commits will require this patch to pass
CI.
The mempool/electrs docker image used in those tests only supports
submitpackage via the esplora interface, not the electrum interface.
We bump the Bitcoin Core version used in kotlin and python tests to
support ephemeral dust. This is required for 0FC channels.
In upcoming commits we will read this knob to determine whether to
negotiate 0FC channels.

For now, we make a best-effort attempt to make sure the configured chain
source supports 0FC channels if this knob is set.

Do this roundtrip at the same time we make a roundtrip to retrieve the
feerates to keep startup as fast as possible.
Implementations of `BroadcasterInterface` cannot assume any topological
ordering on the transactions received, so here we order the received
transactions before adding them to the broadcast queue. Any consumers of
the queue can now assume all transactions received to be topologically
sorted.

Codex wrote the tests.
These will be useful when we add support for broadcasting packages in an
upcoming commit.
We rely on the `BroadcasterInterface` contract whereby any
multi-transaction vector must be a single child and its parents.

In a prior commit, we added the guarantee that any packages received
from the broadcast queue are already topologically sorted, and hence
can be passed directly to the `submit_package` Bitcoin Core RPC.

We avoid broadcasting non-TRUC parents-child packages via
`submitpackage` for now to avoid adding a requirement to support
`submitpackage` for users that don't enable zero-fee commitment
channels. We will do so once support for `submitpackage` is more
ubiquitous.
The electrsd dev-dependency does not yet ship binaries that support
submitpackage on electrum, so we do not turn on 0FC channels in local
tests, and instead only turn them on in a standalone CI workflow, which
includes a custom electrs server that adds support for submitpackage
on electrum.

Co-Authored-By: HAL 9000
Anchor reserve accounting only looked at channels returned by the
ChannelManager. That misses an important force-close transition:
ChannelManager::list_channels can stop returning a channel before
its commitment transaction confirms. During that window the
ChannelMonitor can still broadcast, rebroadcast, or fee-bump the
holder commitment and its anchor spend, so the wallet's emergency
reserve must remain available.

We now require a anchor reserve to be kept for non-trusted channels
where the type is not yet known; we've recently made anchor channels
always supported in ldk-node, so at this point it is safe to assume most
channels of ldk-node will be anchor channels.

We now no longer count existing 0FC channels to validate support
for submitpackage on the chain source if those channels are in the
trusted peers list.

Co-Authored-By: HAL 9000
Store-backed integration tests drive wallet syncs explicitly. Leaving
Esplora background sync enabled let the first background scan start
before the funded address was tracked, so a later sync_wallets call
could observe a zero premine balance.

Reuse the manual Esplora sync config from shared test setup for
Postgres, VSS, migration, and current-version compatibility builders.

Co-Authored-By: HAL 9000
Prober::locked_msat only counted pending probe records from
ChannelManager::list_recent_payments. Across restart, LDK can expose a
still-locked outbound HTLC through channel state before the matching
pending probe record is visible again, which made the restored locked
budget read as zero.

Fall back to unresolved outbound channel HTLCs when no recent pending
probe accounting is available. Also make the restart test retry the
probe-and-restart boundary if the probe failure wins the race before the
node state is persisted.

Co-Authored-By: HAL 9000
@tankyleo

Copy link
Copy Markdown
Contributor Author

These last two commits here are one-shot attempts at fixing some CI issues I ran into, please take them with a grain of salt I have not audited them, let's see if we can get CI to pass here :)

@tnull

tnull commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

These last two commits here are one-shot attempts at fixing some CI issues I ran into, please take them with a grain of salt I have not audited them, let's see if we can get CI to pass here :)

Hmm, so the Esplora fix makes somewhat sense to me, though not quite sure why it's suddenly needed everywhere. The second probing related commit should be unnecessary after #981? If not, we should definitely not introduce this commit here, as it changes completely unrelated behavior. Please drop the commit.

@tnull tnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash.

As mentioned in above/comments, let's please split out the CI fixes and the reserve accounting changes to other PRs. Would also be good to revert all the non-0fc workflows/tests to their prior state as far as possible, just to keep this PR less invasive overall.

Other than that and some minor comments, we should be good to go.

- name: Download bitcoind
if: "matrix.platform != 'windows-latest' && steps.cache-bitcoind.outputs.cache-hit != 'true'"
run: |
source ./scripts/download_bitcoind_electrs.sh

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have the separate 0fc-integration.yml, can we revert all these changes in unrelated workflows to make this PR generally less invasive? Seems like we can just keep the old script and all workflows as-is, and just add a new script that's going to be used just by the 0fc tests workflow?

waitForBlock(esploraEndpoint, lastBlockHash)
}

fun mineChannelClosureUntilSpendable(esploraEndpoint: String, nodes: List<Node>) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why we need all these Kotlin/Python changes?

Comment thread src/liquidity/mod.rs
lsps2_service: Option<LSPS2Service>,
wallet: Arc<Wallet>,
channel_manager: Arc<ChannelManager>,
chain_monitor: Arc<ChainMonitor>,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I'm the biggest fan of wiring-through ChainMonitor everywhere. Maybe it would be better to have a dedicated reserve-tracker object for this rather than a util that requires an increasing number of parameters. It could live somewhere under src/channel (cf #946, where we'll probably add a store for channels under that path) or maybe balance.rs?

In any case, it seems like a more substantial change than expected, can we pull the reserve-calculation updates out to a separate (pre-factor or follow-up, your choice) PR?

Comment thread src/tx_broadcaster.rs
fn new(txs: &[(&Transaction, LdkTransactionType)]) -> Self {
Self(txs.iter().map(|(tx, tx_type)| ((*tx).clone(), Some(tx_type.clone()))).collect())
let (txs, types) =
txs.iter().map(|(tx, tx_type)| ((*tx).clone(), Some(tx_type.clone()))).unzip();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think unzip also allocates, so not sure this change really saves us anything.

Given the two side-by-side Vecs are easier to misuse/get out of sync compared to a single Vec of tuples, maybe this might be worth reverting?

Comment thread src/config.rs
/// the channel, which eliminates a very common source of channel force-closures. These
/// channels instead source *all* the fees required to confirm the commitment from the
/// anchor reserve of the channel closer at the time of force-close. If set, your chain
/// source *must* support the `submitpackage` Bitcoin Core RPC, and relay TRUC, P2A, and

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can we link to resources for these? I.e., what's "TRUC", "P2A", and "ephemeral dust"?

Comment thread src/probing.rs
// before it exposes the matching pending probe payment again. ChannelDetails does
// not identify probe HTLCs, so fall back conservatively when no recent probe
// accounting is available.
self.channel_manager

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fallback seems brittle and may mask an upstream accounting or persistence-reconciliation issue? I think it may also undercount when one probe is reported but others are missing.

https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4806 is exposing a bit more info, that could be used to exclude at least the forwards in this fallback.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way the fix doesn't belong in this PR. Rather, would be good to add over at #975 (maybe before checking whether the linked comments already address parts of it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Goal: Merge

Development

Successfully merging this pull request may close these issues.

7 participants